@alex-rawlings-yyc resolved 4 discussions.
Reviewable status: 0 of 56 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).

@alex-rawlings-yyc resolved 1 discussion.
Reviewable status: 0 of 70 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).

@imnasnainaec partially reviewed 53 files and made 1 comment.
Reviewable status: 50 of 73 files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc).

.github/workflows/test.yml line 114 at r9 (raw file):

            if ($LASTEXITCODE -eq 0) { exit 0 }
            Write-Host "npm ci failed (exit $LASTEXITCODE); verifying cache before retry"
            npm cache verify

Claude suggestion from a Devin nit:

worth a one-liner after npm cache verify (e.g., if ($LASTEXITCODE -ne 0) { Write-Host "npm cache verify failed (exit $LASTEXITCODE)" } for clearer diagnostics)

@alex-rawlings-yyc resolved 1 discussion.
Reviewable status: 50 of 73 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc and imnasnainaec).

npm run test:e2e passes for me locally on Windows 11 with the latest paranext-core (3eb6556cfd2e37d1610c3fcf873a08ed4892f635). I just have a handful of nit-picks so far and should finish reviewing tomorrow afternoon.

@imnasnainaec reviewed 15 files and made 6 comments.
Reviewable status: 65 of 73 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).

.github/workflows/test.yml line 102 at r12 (raw file):

        working-directory: paranext-core
        shell: pwsh
        # Same install as Linux (no --omit=optional: lightningcss native binaries are optional deps

⛏️ Unnecessary parenthetical duplicated from above.

e2e-tests/playwright.config.ts line 23 at r12 (raw file):

  retries: process.env.CI ? 2 : 1,
  workers: 1,
  // Tier-specific report/output folders so `npm run test:e2e` (smoke then cdp, sequentially) doesn't

⛏️ The first sentence of this comment feels unnecessary.

e2e-tests/README.md line 12 at r12 (raw file):

Both tiers are self-launching: the CDP tier's `globalSetup` launches its own Platform.Bible instance (with `--remote-debugging-port=9223`) in an isolated user-data dir and tears it down afterward, so `npm run test:e2e:cdp` needs no manual `npm run start:cdp` first. To iterate against a warm instance instead, run `npm run start:cdp` in one terminal, then run the CDP config directly with `npx playwright test --config e2e-tests/playwright-cdp.config.ts`: the setup detects the in-use CDP port, reuses that instance, and leaves it running.

In CI (`.github/workflows/test.yml`, `e2e` job) the full suite runs on both Linux and Windows. Each tier writes its Playwright HTML report to its own subfolder (`playwright-report/smoke`, `playwright-report/cdp`) so a combined run keeps both.

⛏️ The sentence about subfolders feels superfluous here in the README.

e2e-tests/README.md line 39 at r12 (raw file):

- **The instance is only ever used with the WEB project.** This is an operating assumption, not something tests verify: `ensureInterlinearizerOpenOnWeb()` trusts an existing Interlinearizer tab and only picks WEB when opening fresh. Don't point `start:cdp` at other projects.
- **Mutating tests operate on the dedicated "E2E Test Project", never on a developer's own projects.** `ensureE2eProjectActive()` opens it (creating it on first use) at the start of each mutating test. Because the draft is the single per-source working buffer, replacing it could destroy unsaved developer work — so when the draft is dirty and the active project is _not_ the e2e project, the helper first saves the draft into a new `e2e-rescued-work-<timestamp>` project. Rescue projects are backups, not junk: delete them manually once recovered. Dirty state left while the e2e project is active is treated as leftover test data and discarded.
- **Self-establish every precondition.** Each mutating test starts with `ensureInterlinearizerOpenOnWeb()` → `ensureE2eProjectActive()` → `navigateToScriptureRef()` (the scroll-group reference could be anywhere) → `wipeDraft()`.

⛏️ If we're documenting that every test of a given class begins with a chain of 4 functions, perhaps we should wrap those in a single function.

e2e-tests/README.md line 40 at r12 (raw file):

- **Mutating tests operate on the dedicated "E2E Test Project", never on a developer's own projects.** `ensureE2eProjectActive()` opens it (creating it on first use) at the start of each mutating test. Because the draft is the single per-source working buffer, replacing it could destroy unsaved developer work — so when the draft is dirty and the active project is _not_ the e2e project, the helper first saves the draft into a new `e2e-rescued-work-<timestamp>` project. Rescue projects are backups, not junk: delete them manually once recovered. Dirty state left while the e2e project is active is treated as leftover test data and discarded.
- **Self-establish every precondition.** Each mutating test starts with `ensureInterlinearizerOpenOnWeb()` → `ensureE2eProjectActive()` → `navigateToScriptureRef()` (the scroll-group reference could be anywhere) → `wipeDraft()`.
- **Reset at the start, tidy at the end.** The start-of-test sequence is what guarantees correctness — it self-heals whatever a failed run left behind. Part of that self-heal is automatic: `ensureInterlinearizerOpenOnWeb()` calls `dismissLeftoverModals()`, which cancels any modal a timed-out prior test left mounted — a project modal's overlay is `fixed inset-0 z-50` and would otherwise intercept every click in the tests that follow, turning one real failure into a cascade. This is also why the CDP config can safely `retries` in CI: the retry lands on a self-healed instance, not on the overlay the failed attempt left up. Mutating tests additionally end with `ensureE2eProjectActive(page, { rescueDirtyDraft: false })` to discard their own leftovers; this is a courtesy so the next run doesn't misread test junk as rescuable developer work, not something correctness depends on.

⛏️ The full details of the reset-at-start and tidy-at-end should be in the functions' JSDocs. A more general description would be more fitting here.

@alex-rawlings-yyc made 5 comments.
Reviewable status: 65 of 73 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).

.github/workflows/test.yml line 102 at r12 (raw file):

Done. Trimmed the parenthetical. The "Same install as Linux" opener already points back to the Linux comment above, so the --omit=optional/lightningcss detail only lives in one place now.

e2e-tests/playwright.config.ts line 23 at r12 (raw file):

Done. Dropped the "smoke then cdp, sequentially" lead-in. The tier-specific report/output folders rationale reads fine on its own.

e2e-tests/README.md line 12 at r12 (raw file):

Done. Removed the subfolder sentence from the CI paragraph. That detail now lives only in the playwright.config.ts comment where the folders are actually configured.

e2e-tests/README.md line 39 at r12 (raw file):

I'd rather keep these as separate calls than wrap them. The chain isn't actually uniform: the draft-persistence reopen (draft-persistence.spec.ts:54-55) deliberately runs only ensureInterlinearizerOpenOnWeb() + navigateToScriptureRef(), skipping ensureE2eProjectActive() and wipeDraft() — re-establishing the project or wiping the draft would destroy the exact state that test is verifying. A single wrapper would hide that meaningful subset-selection. I've reworded the bullet to say the steps compose as needed and to call out the reopen case, so the "why not one function" is documented. Happy to reconsider if you still want a wrapper for the common all-four setup.

e2e-tests/README.md line 40 at r12 (raw file):

Agreed. Condensed the bullet to a general description that points at the JSDocs on ensureInterlinearizerOpenOnWeb() and ensureE2eProjectActive(). Those already carry the full reset-at-start / tidy-at-end mechanism (the modal/picker self-heal and the rescueDirtyDraft semantics), so nothing was lost in the trim.

@imnasnainaec reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: 69 of 73 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).

e2e-tests/global-teardown-cdp.ts line 23 at r13 (raw file):

  // need it fully dead before removing its user-data dir below.
  let appKilled = false;
  if (fs.existsSync(CDP_PID_FILE)) {

⛏️ Since the content of this if block is nearly identical to the corresponding block in e2e-tests/global-teardown.ts, could it be extracted as an exported function there that can be imported and used here?

e2e-tests/tests/features/draft-persistence.spec.ts line 25 at r13 (raw file):

    // (test.slow() above triples only the overall test timeout for the two open-cycles below, not
    // this startup readiness check.)
    await waitForAppAndInterlinearizerReady(mainPage, {

⛏️ Since the same options (and similar justifying comments) are present in every features/ use of waitForAppAndInterlinearizerReady, can we consider simplifying it to a single bool param (e.g., lenient or cdp)? Then all these comments and the docs on the options type could all be absorbed into the docs for the single param.

@imnasnainaec addressed nits; both worthwhile adjustments.

@alex-rawlings-yyc made 1 comment.
Reviewable status: 69 of 73 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).